GH-15233: [C++] Fix CopyFiles when destination is a FileSystem with background_writes #44897

Tom-Newton · 2024-12-02T11:30:02Z

Rationale for this change

Currently it deadlocks when trying to upload more files than we have IO threads.

What changes are included in this PR?

Use CloseAsync() inside copy_one_file so that it returns futures, instead of blocking while it waits for other tasks to complete. This avoids the deadlock.
Fix a segfault in FileInterface::CloseAsync() by using shared_from_this() to so that it doesn't get destructed prematurely. shared_from_this() is already used in the CloseAsync() implementation on several filesystems. After change 1 this is important when downloading to the local filesystem using CopyFiles.
Spawn copy_one_file less urgently than default, so that background_writes are done with higher priority. Otherwise copy_one_file will keep buffering more data in memory without giving the background_writes any chance to upload the data and drop it from memory. Therefore, without this large copies would cause OOMs.
1. The current Arrow thread pool implementation makes provision for spawning with a set priority but the priority numbers are just ignored.
2. I made a small change to use a std::priority_queue to implement the priority.
3. There is a property of the current implementation that tasks will be scheduled in the order of calls to Spawn. There are also a few test cases in arrow-acero-hash-aggregate-test that fail if this property is not maintained. I'm not sure what is the correct option but for now I have ensured that tasks of the same priority are run in the order they are spawned.
4. I think this has got to be the most contentious part of the change, with the broadest possible impact if I introduce a bug. I would appreciate some input on whether this seems like a reasonable change and I'm very happy to move it to a separate PR and/or discuss further.

Are these changes tested?

Added some extra automated tests. I also ran some large scale tests copying between Azure and local with a directory of about 50,000 files of varying sizes totalling 160GiB.

Are there any user-facing changes?

CopyFiles should now work reliably
ThreadPool now runs tasks in priority order

GitHub Issue: [Python] pyarrow.fs.copy_files hangs indefinitely #15233

cpp/src/arrow/filesystem/filesystem.cc

pitrou

This looks good on the principle.

By the way, this seems based on a rather old version of git main, can you update?

cpp/src/arrow/util/thread_pool.cc

cpp/src/arrow/util/thread_pool_test.cc

cpp/src/arrow/io/interfaces.cc

cpp/src/arrow/filesystem/test_util.cc

cpp/src/arrow/filesystem/filesystem.cc

Tom-Newton

I rebased on the latest main and I think I've addressed all the comments

cpp/src/arrow/util/thread_pool.cc

cpp/src/arrow/filesystem/test_util.cc

pitrou

Thanks for the update! Here are some more comments.

cpp/src/arrow/filesystem/test_util.cc

cpp/src/arrow/util/thread_pool.cc

cpp/src/arrow/util/thread_pool_test.cc

pitrou

+1, let's wait for CI

pitrou · 2024-12-10T08:18:27Z

@github-actions crossbow submit -g cpp

pitrou · 2024-12-10T09:01:17Z

Hmm, it looks like we'll either have to fix the serial executor to also take priorities into account (if that's at all possible?), or simply disable the priority test when threading is disabled.

Tom-Newton · 2024-12-11T17:17:18Z

Hmm, it looks like we'll either have to fix the serial executor to also take priorities into account (if that's at all possible?), or simply disable the priority test when threading is disabled.

I think we can implement priorities exactly the same on SerialExecutor as on ThreadPool, so I pushed a commit to do that. I feel like this should be covered by the normal CI though, without needing to build with ARROW_ENABLE_THREADING=OFF, so I'm going to see how easy it is to parameterise the new tests I added.

Tom-Newton · 2024-12-11T17:36:05Z

I feel like this should be covered by the normal CI though, without needing to build with ARROW_ENABLE_THREADING=OFF, so I'm going to see how easy it is to parameterise the new tests I added.

I think it would at least require making the constructor of SerialExecutor public, so maybe its not really desirable to add such a test?

…dlocks

…test

Co-authored-by: Antoine Pitrou <[email protected]>

Tom-Newton · 2024-12-13T16:20:53Z

@pitrou do you have any opinion on extra tests for priorities in SerialExecutor? If you are happy please can you trigger the crossbow tests again.

pitrou

+1, just a nit

cpp/src/arrow/util/thread_pool.cc

pitrou · 2024-12-16T11:31:11Z

@github-actions crossbow submit -g cpp

Tom-Newton · 2024-12-16T11:56:53Z

Sorry, it looks like there are some bits that I missed when converting SerialExecutor to priority queue because I never tested it locally with ARROW_ENABLE_THREADING=OFF

Tom-Newton · 2024-12-16T13:45:15Z

Ok, this time I managed to test it locally ARROW_ENABLE_THREADING=OFF so hopefully I haven't missed anything else.

pitrou · 2024-12-16T14:54:48Z

@github-actions crossbow submit -g cpp

github-actions · 2024-12-16T14:57:30Z

Revision: 5774bf2

Submitted crossbow builds: ursacomputing/crossbow @ actions-372e4c30db

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-20.04-cuda-11.2.2
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

Tom-Newton · 2024-12-16T15:15:43Z

Thanks for triggering the extra tests again. That run is looking better but it looks like a couple failed because of a timeout downloading a dependency

    error: downloading 'https://archive.apache.org/dist/orc/orc-format-1.0.0/orc-format-1.0.0.tar.gz' failed
          status_code: 28
          status_string: "Timeout was reached"
          log:
          --- LOG BEGIN ---
            Trying 65.108.204.189:443...
    Trying 2a01:4f9:1a:a084::2:443...

Presumably this is not related to me changes.

pitrou · 2024-12-16T16:34:18Z

Yes, I've restarted those CI builds.

pitrou · 2024-12-16T21:35:29Z

Ok, thanks a lot for doing this @Tom-Newton !

Tom-Newton · 2024-12-16T21:39:10Z

Ok, thanks a lot for doing this @Tom-Newton !

Thanks for reviewing

conbench-apache-arrow · 2025-01-10T00:07:48Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 894f5fb.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.

github-actions bot added Component: C++ awaiting review Awaiting review labels Dec 2, 2024

Tom-Newton mentioned this pull request Dec 3, 2024

[Python] pyarrow.fs.copy_files hangs indefinitely #15233

Closed

pitrou reviewed Dec 4, 2024

View reviewed changes

cpp/src/arrow/filesystem/filesystem.cc Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Dec 4, 2024

pitrou requested changes Dec 4, 2024

View reviewed changes

Tom-Newton force-pushed the tomnewton/fix_pyarrow_copy_files_uploads/GH-15233 branch from 1294e38 to dbefb7e Compare December 4, 2024 14:49

Tom-Newton commented Dec 4, 2024

View reviewed changes

cpp/src/arrow/util/thread_pool.cc Show resolved Hide resolved

cpp/src/arrow/filesystem/test_util.cc Outdated Show resolved Hide resolved

cpp/src/arrow/filesystem/test_util.cc Outdated Show resolved Hide resolved

Tom-Newton marked this pull request as ready for review December 4, 2024 14:54

Tom-Newton requested a review from pitrou December 9, 2024 13:18

pitrou reviewed Dec 9, 2024

View reviewed changes

cpp/src/arrow/filesystem/test_util.cc Outdated Show resolved Hide resolved

cpp/src/arrow/util/thread_pool.cc Outdated Show resolved Hide resolved

cpp/src/arrow/util/thread_pool_test.cc Outdated Show resolved Hide resolved

pitrou approved these changes Dec 10, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

Tom-Newton added 12 commits December 12, 2024 22:28

Add Generic filesytem test for CopyFiles

2663131

Use CloseAsync when copying each file to avoid wasted threads and dea…

c7e67e4

…dlocks

Fix segfault in FileInterface::CloseAsync by using a shared pointer

8058f42

Actually use TaskHints.priority in ThreadPool and add a test

8c81ede

Run copy file tasks with lower priority

07e09e6

Assert futures are still in submitted order

9ce8deb

Ensure tasks of the same priority run in their spawned order and add …

d12567d

…test

CI tidy up

f0d4bd0

Add comment

d2812b6

Address several PR comments

9b5d1e0

Minor tidy ups

3b34a6b

Autoformat

6934b60

Tom-Newton and others added 3 commits December 12, 2024 22:28

Update cpp/src/arrow/filesystem/test_util.cc

6de2952

Co-authored-by: Antoine Pitrou <[email protected]>

Fix PR comments

e3caae7

Implement queuing for SerialExecutor too

7573d11

Tom-Newton force-pushed the tomnewton/fix_pyarrow_copy_files_uploads/GH-15233 branch from 697c747 to 7573d11 Compare December 12, 2024 22:28

Tom-Newton force-pushed the tomnewton/fix_pyarrow_copy_files_uploads/GH-15233 branch from 93c00e9 to 7573d11 Compare December 13, 2024 16:31

pitrou approved these changes Dec 16, 2024

View reviewed changes

cpp/src/arrow/util/thread_pool.cc Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

Fix with threading disabled

5774bf2

Rename spawned_tasks_count -> spawned_tasks_count_

c627f04

pitrou changed the title ~~GH-15233: Fix CopyFiles when destination is a FileSystem with background_writes~~ GH-15233: [C++] Fix CopyFiles when destination is a FileSystem with background_writes Dec 16, 2024

pitrou merged commit 894f5fb into apache:main Dec 16, 2024
38 checks passed

pitrou removed the awaiting committer review Awaiting committer review label Dec 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-15233: [C++] Fix CopyFiles when destination is a FileSystem with background_writes #44897

GH-15233: [C++] Fix CopyFiles when destination is a FileSystem with background_writes #44897

Tom-Newton commented Dec 2, 2024 •

edited by pitrou

Loading

pitrou left a comment

Tom-Newton left a comment •

edited

Loading

pitrou left a comment

pitrou left a comment

pitrou commented Dec 10, 2024

This comment was marked as outdated.

pitrou commented Dec 10, 2024

Tom-Newton commented Dec 11, 2024

Tom-Newton commented Dec 11, 2024 •

edited

Loading

Tom-Newton commented Dec 13, 2024 •

edited

Loading

pitrou left a comment

pitrou commented Dec 16, 2024

This comment was marked as outdated.

Tom-Newton commented Dec 16, 2024

Tom-Newton commented Dec 16, 2024

pitrou commented Dec 16, 2024

github-actions bot commented Dec 16, 2024

Tom-Newton commented Dec 16, 2024

pitrou commented Dec 16, 2024

pitrou commented Dec 16, 2024

Tom-Newton commented Dec 16, 2024

conbench-apache-arrow bot commented Jan 10, 2025

GH-15233: [C++] Fix CopyFiles when destination is a FileSystem with background_writes #44897

GH-15233: [C++] Fix CopyFiles when destination is a FileSystem with background_writes #44897

Conversation

Tom-Newton commented Dec 2, 2024 • edited by pitrou Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

pitrou left a comment

Choose a reason for hiding this comment

Tom-Newton left a comment • edited Loading

Choose a reason for hiding this comment

pitrou left a comment

Choose a reason for hiding this comment

pitrou left a comment

Choose a reason for hiding this comment

pitrou commented Dec 10, 2024

This comment was marked as outdated.

pitrou commented Dec 10, 2024

Tom-Newton commented Dec 11, 2024

Tom-Newton commented Dec 11, 2024 • edited Loading

Tom-Newton commented Dec 13, 2024 • edited Loading

pitrou left a comment

Choose a reason for hiding this comment

pitrou commented Dec 16, 2024

This comment was marked as outdated.

Tom-Newton commented Dec 16, 2024

Tom-Newton commented Dec 16, 2024

pitrou commented Dec 16, 2024

github-actions bot commented Dec 16, 2024

Tom-Newton commented Dec 16, 2024

pitrou commented Dec 16, 2024

pitrou commented Dec 16, 2024

Tom-Newton commented Dec 16, 2024

conbench-apache-arrow bot commented Jan 10, 2025

Tom-Newton commented Dec 2, 2024 •

edited by pitrou

Loading

Tom-Newton left a comment •

edited

Loading

Tom-Newton commented Dec 11, 2024 •

edited

Loading

Tom-Newton commented Dec 13, 2024 •

edited

Loading